Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed empty event set #59

Merged
merged 3 commits into from
Feb 27, 2019
Merged

Fixed empty event set #59

merged 3 commits into from
Feb 27, 2019

Conversation

inammathe
Copy link
Contributor

@inammathe inammathe commented Jan 24, 2019

This has been bothering me (and likely many other users) since its introduction in #57

It used to be that the event.set(xpath_dest, data) was protected by existing after the return if value.is_a?(Array) && value.length == 0 (which is now a next) and within the normalized_nodeset.each loop

Now, data is initialized and event.set(xpath_dest, data) occurs regardless of what happens within the loop. The result is empty event fields being set where in the past, nothing would be set.

The repercussions are pretty significant for anyone relying on xml xpath results for subsequent filter plugins e.g. geoip

So far this change has forced me to use methods similar to this literally anytime I dare to use xml and the cost is unnecessary.

ruby {
    code => "event.remove('FieldName') if event.get('[FieldName]').empty?"
}

No testing has been been performed on this change (except for the included rspec test which is passing), however given the above explanation the logic is pretty straight forward.
e.g.

force_array = false
data = []

if data.size == 1 && !force_array
    puts true
else
    puts false unless data.empty?
end

^ shouldn't return anything

Solves #61

@inammathe
Copy link
Contributor Author

those CI failures have been around for a little while it seems :)

@elasticcla
Copy link

Hi @inammathe, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jsvd jsvd merged commit a4623f7 into logstash-plugins:master Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants